Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ENH: Pad or crop volume using scil_volume_reshape.py #1019

Merged
merged 10 commits into from
Oct 8, 2024
Merged

ENH: Pad or crop volume using scil_volume_reshape.py #1019

merged 10 commits into from
Oct 8, 2024

Conversation

AntoineTheb
Copy link
Contributor

@AntoineTheb AntoineTheb commented Jul 31, 2024 •
edited
Loading

Quick description

This new script allows to pad or crop a volume to a specified resolution (or to match a reference volume). It is complementary to scil_volume_resample.py.

This will be useful to build machine learning datasets where it is desirable to deal with fixed-size volumes.

Type of change

Check the relevant options.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Provide data, screenshots, command line to test (if relevant)

GDrive

fodf_128.nii.gz was used as the base file.

image
image
image
image

Checklist

  • My code follows the style guidelines of this project (run autopep8)
  • I added relevant citations to scripts, modules and functions docstrings and descriptions
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I moved all functions from the script file (except the argparser and main) to scilpy modules
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@AntoineTheb AntoineTheb self-assigned this Jul 31, 2024
@AntoineTheb AntoineTheb marked this pull request as ready for review July 31, 2024 19:53
@AntoineTheb AntoineTheb changed the title WIP: Pad or crop volume using scil_volume_reshape.py ENH: Pad or crop volume using scil_volume_reshape.py Jul 31, 2024
@codecov Codecov
Copy link

codecov bot commented Jul 31, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 91.04478% with 6 lines in your changes missing coverage. Please review.

Project coverage is 68.84%. Comparing base (1d6f848) to head (7bae331).
Report is 11 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #1019      +/-   ##
==========================================
+ Coverage   68.78%   68.84%   +0.06%     
==========================================
  Files         429      431       +2     
  Lines       22265    22332      +67     
  Branches     3324     3332       +8     
==========================================
+ Hits        15314    15375      +61     
- Misses       5665     5670       +5     
- Partials     1286     1287       +1
Components Coverage Δ
Scripts 69.70% <85.36%> (+0.04%) ⬆️
Library 67.63% <100.00%> (+0.09%) ⬆️

@arnaudbore
Copy link
Contributor

Quick discussion with @frheault, can you rename scil_volume_reshape_to_reference.py to scil_volume_reslice_to_reference.py ?

@AntoineTheb
Copy link
Contributor Author

Good point @arnaudbore, done in 0e925ed

EmmaRenauld
EmmaRenauld approved these changes Aug 6, 2024
View reviewed changes
Copy link
Contributor

@EmmaRenauld EmmaRenauld left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super clear. Thanks for the tests!

frheault
frheault reviewed Aug 15, 2024
View reviewed changes
Copy link
Member

@frheault frheault left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything seems to be working well. I don't have much to say, thanks for renaming the reshape_to_reference for reslice_to_reference.

scripts/scil_volume_reshape.py Outdated Show resolved Hide resolved
scripts/scil_volume_reshape.py Outdated Show resolved Hide resolved
scilpy/image/volume_operations.py
affine = img.affine

# Compute the difference between the desired shape and the current shape
diff = (np.array(volume_shape) - np.array(data.shape[:3])) // 2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How hard would it be to have an option to be centered on the data's barycenter?
Would it be useful to allow to pad or crop even if the brain is not exactly center?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it could be a nice option but kind of out of scope for this PR. One way to do this without any modification would be to use scil_volume_crop, then pad to the desired shape.

scilpy/image/tests/test_volume_operations.py Show resolved Hide resolved
@AntoineTheb
Copy link
Contributor Author

image
@arnaudbore ?

@AntoineTheb
Copy link
Contributor Author

Really for merging @arnaudbore @frheault

@frheault frheault merged commit 8b67a06 into scilus:master Oct 8, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@EmmaRenauld EmmaRenauld EmmaRenauld approved these changes

@frheault frheault frheault left review comments

@elyz081 elyz081 elyz081 approved these changes

@arnaudbore arnaudbore Awaiting requested review from arnaudbore

Assignees

@AntoineTheb AntoineTheb

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@AntoineTheb @arnaudbore @EmmaRenauld @frheault @elyz081